Clean workers too from the clean command #3579
Conversation
This comment was marked as outdated.
This comment was marked as outdated.
alexarchambault
force-pushed
the
clean-workers
branch
from
af3816e
to
1b9f647
Compare
This comment was marked as resolved.
This comment was marked as resolved.
|
kicked it |
| @@ -21,7 +21,14 @@ trait Evaluator { | |||
| def outPath: os.Path | |||
| def externalOutPath: os.Path | |||
| def pathsResolver: EvaluatorPathsResolver | |||
| // TODO In 0.13.0, workerCache should have the type of mutableWorkerCache, | |||
| // while the latter should be removed | |||
| def workerCache: collection.Map[Segments, (Int, Val)] | |||
There was a problem hiding this comment.
Althouh this must stay for compat, we already can deprecate it in favor of its replacement. We try to deprecate API we intend to remove, to provide a smoother migration path.
There was a problem hiding this comment.
I was imagining a simple change to def workerCache: collection.mutable.Map[Segments, (Int, Val)] in 0.13, and a removal of mutableWorkerCache, that should only be used internally. It's a really small change.
Are there know users of the Evaluator trait, that don't rely on the Mill internal implementation? Implementing that seems more of an internal thing to me… (and for users of Evaluator, given that collection.mutable.Map <: collection.Map, the change should be source-compatible).
There was a problem hiding this comment.
For now the policy is that the things that are exposed we treat as stable for bincompat. We don't actually know if anyone uses it, but if we expect something to be internal-only we'll mark them as private[mill] and MIMA will let us do as we like
There was a problem hiding this comment.
The current state of the PR doesn't break bin compat. But the question is, should this (minor and mostly source-compatible) change go through a deprecation cycle, or should we do it in one go in 0.13? A change in one go, for this particular minor change, looks fine to me.
alexarchambault
force-pushed
the
clean-workers
branch
2 times, most recently
from
f02b53a
to
1522e43
Compare
alexarchambault
force-pushed
the
clean-workers
branch
from
1522e43
to
b6e896e
Compare
There was a problem hiding this comment.
Last things before merging, apart from those already mentioned in #3580:
-
Can we make
rm -rfclean the workers as well? If we leave amodule-name/worker-name.jsonfile on disk as I suggested in the other PR, we could compare it to what's in memory, and anrm -r out/module-namewould invalidate the worker on next run even without amill clean. Although not strictly necessary since people "should" be usingcleaninstead ofrm -rf, keeping both approaches working would help maintain the symmetry between theoutfolder and the module/task hierarchy -
Update PR description
alexarchambault
force-pushed
the
clean-workers
branch
from
b6e896e
to
d155578
Compare
This PR ensures the
cleancommand works for worker tasks, and allows users to dispose of workers by removing their corresponding metadata file on disk (out/foo/theWorker.jsonfor workerfoo.theWorker) - in that case, the worker instance is dropped upon first access to it.Fixes #3276